BI-4975: Construct file-connector s3 path dynamically #31
Conversation
asnytin
force-pushed
the
asnytin/s3_filename_suffix
branch
from
e1b1fcf to
23b2d1e
Compare
lib/dl_connector_bundle_chs3/dl_connector_bundle_chs3_tests/db/base/api/connection.py
Outdated
Show resolved
Hide resolved
lib/dl_connector_bundle_chs3/dl_connector_bundle_chs3_tests/db/conftest.py
Outdated
Show resolved
Hide resolved
| def get_full_s3_filename(self, s3_filename_suffix: Optional[str]) -> Optional[str]: | ||
| if s3_filename_suffix is None: | ||
| return None |
There was a problem hiding this comment.
Why Optional (both in arguments and in return type)? I can't see any calls where either can be None (except for tests)
There was a problem hiding this comment.
Right now s3_filename_suffix will be loaded as None for old connections until migration.
After migration Optional should be removed (with this fallback
There was a problem hiding this comment.
Yes, exactly, it is tested against being None here in the line 120 before the get_full_s3_filename call
There was a problem hiding this comment.
okay, you are right, I can remove Optionals from the method. But actually it's just a copy of old property src.s3_filename signature that can be None if save task haven't succeeded yet.
There was a problem hiding this comment.
Just a reminder for all of us, that this check will have to be moved above the fallback after the latter is removed
asnytin
force-pushed
the
asnytin/s3_filename_suffix
branch
from
23b2d1e to
ef3cd86
Compare
lib/dl_connector_bundle_chs3/dl_connector_bundle_chs3_tests/db/conftest.py
Outdated
Show resolved
Hide resolved
No description provided.